Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build one MonoTargetsTasks assembly for Microsoft.NET.Runtime.MonoTargets.Sdk pack #59720

Merged
merged 15 commits into from
Sep 30, 2021

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Sep 28, 2021

Instead of 3 assemblies for some tiny tasks.

Building a single assembly also ensures that everything in the pack depends on and bundles identical versions of dependent assemblies. For example on net472, a single version of System.Text.Json with a single fixed version shared by both the JsonToItemsTaskFactory task factory and the RuntimeConfigParser task.

Related to #59619

Additional fixes:

  • Update all the READMEs
  • Build AssemblyStripper.dll that just includes CilStrip. This is primarily to turn off nullability and code style checking just for the old Mono.Cecil assemblies.
  • Don't include Microsoft.Build.dll with the net472 version of the tasks - there is no need to ship an extra copy of MSBuild.
  • Update ILStrip to use the same Parallel.ForEach logic as MonoAOTCompiler

@ghost
Copy link

ghost commented Sep 28, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Instead of 3 assemblies for some tiny tasks.

Building a single assembly also ensures that everything in the pack depends on and bundles identical versions of dependent assemblies. For example on net472, a single version of System.Text.Json with a single fixed version shared by both the JsonToItemsTaskFactory task factory and the RuntimeConfigParser task.

Related to #59619


todo:

  • Fix all the READMEs
  • Build CilStrip to a separate helper assembly (but keep ILStripTask in MonoTargetsTasks.dll) with nullability disabled, and re-enable nullability for the tasks assembly
  • Figure out why net472 includes a copy of Microsoft.Build.dll instead of just referencing it.
Author: lambdageek
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

@lambdageek lambdageek changed the title Build one MonoTargetsTasks assembly for Mono.NET.Runtime.MonoTargets.Sdk pack Build one MonoTargetsTasks assembly for Microsoft.NET.Runtime.MonoTargets.Sdk pack Sep 28, 2021
@lambdageek lambdageek force-pushed the single-mono-targets-tasks-assembly branch 3 times, most recently from 2c13699 to a6a0798 Compare September 29, 2021 04:02
…Sdk pack

Instead of 3 assemblies for some tiny tasks.

Building a single assembly also ensures that everything in the pack depends on
and bundles identical versions of dependent assemblies.  For example on net472,
a single version of System.Text.Json with a single fixed version shared by both
the JsonToItemsTaskFactory task factory and the RuntimeConfigParser task.

Related to dotnet#59619
@lambdageek lambdageek force-pushed the single-mono-targets-tasks-assembly branch from a6a0798 to 4ae1a32 Compare September 29, 2021 15:27
Build it with nullability checking disabled.

Build the main MonoTargetsTasks assembly with nullability enabled
In particular, pick up all the dependent assemblies for the net472 build
@lambdageek lambdageek marked this pull request as ready for review September 29, 2021 19:03
@lambdageek
Copy link
Member Author

With these changes, this is what the Microsoft.NET.Runtime.MonoTargets.Sdks.nupkg looks like:

Archive:  artifacts/packages/Release/Shipping/Microsoft.NET.Runtime.MonoTargets.Sdk.7.0.0-ci.nupkg
  Length      Date    Time    Name
---------  ---------- -----   ----
      527  09-29-2021 15:07   _rels/.rels
     1117  09-29-2021 15:07   Microsoft.NET.Runtime.MonoTargets.Sdk.nuspec
       88  07-09-2021 15:00   build/Microsoft.NET.Runtime.MonoTargets.Sdk.props
     7006  09-13-2021 17:01   Icon.png
     1116  01-21-2020 18:43   LICENSE.TXT
       95  09-29-2021 17:42   Sdk/Sdk.props
      105  07-09-2021 15:00   Sdk/Sdk.targets
     1478  09-29-2021 18:04   Sdk/MonoTargetsTasks.props
    10473  07-30-2021 20:59   Sdk/RuntimeComponentManifest.targets
   357376  09-29-2021 19:07   tasks/net472/AssemblyStripper.dll
    20872  10-19-2020 18:40   tasks/net472/Microsoft.Bcl.AsyncInterfaces.dll
    29696  09-29-2021 19:07   tasks/net472/MonoTargetsTasks.dll
    20856  02-19-2020 10:05   tasks/net472/System.Buffers.dll
   189312  10-19-2020 18:37   tasks/net472/System.Collections.Immutable.dll
   141184  02-19-2020 10:05   tasks/net472/System.Memory.dll
   115856  05-15-2018 13:29   tasks/net472/System.Numerics.Vectors.dll
   462728  10-19-2020 18:45   tasks/net472/System.Reflection.Metadata.dll
    16768  10-19-2020 18:46   tasks/net472/System.Runtime.CompilerServices.Unsafe.dll
    65928  10-19-2020 18:40   tasks/net472/System.Text.Encodings.Web.dll
   355712  10-19-2020 18:42   tasks/net472/System.Text.Json.dll
    25984  02-19-2020 10:05   tasks/net472/System.Threading.Tasks.Extensions.dll
    25232  05-15-2018 13:29   tasks/net472/System.ValueTuple.dll
   357888  09-29-2021 19:07   tasks/net6.0/AssemblyStripper.dll
    29184  09-29-2021 19:07   tasks/net6.0/MonoTargetsTasks.dll
    54772  09-20-2021 14:01   THIRD-PARTY-NOTICES.TXT
      713  09-29-2021 15:07   [Content_Types].xml
      677  09-29-2021 15:07   package/services/metadata/core-properties/20a1d10c0d2541d680d1da9fd5359eb4.psmdcp
---------                     -------
  2292743                     27 files

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. It would be useful to try this out with VS though, if possible.

Co-authored-by: Ankit Jain <radical@gmail.com>
@@ -0,0 +1,24 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate project for the AssemblyStripper if it's just a library and can't be part of the MonoTargetsTasks.csproj?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ViktorHofer because it pulls in the CilStripper sources which are not actively maintained, full of warnings and aren't nullability friendly. Putting everything in one assembly means we don't get nice checking for all the other tasks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. AFAIK the old sources will be replaced anyway with a new tooling during NET7.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. AFAIK the old sources will be replaced anyway with a new tooling during NET7.

That's my understanding, too. At that point this extra assembly can go away

@lambdageek lambdageek added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 30, 2021
@lambdageek
Copy link
Member Author

Don't merge, for now.

Need to check that xamarin-macios doesn't depend on the ILStrip assembly name.

@lambdageek
Copy link
Member Author

lambdageek commented Sep 30, 2021

XI does depend on the assembly name in xamarin/xamarin-macios#12563.
So if we move the ILStrip task to another assembly, the XI code will need to change in net7 to use the new name. /cc @chamons

@lambdageek lambdageek removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 30, 2021
@chamons
Copy link

chamons commented Sep 30, 2021

Talked to @rolfbjarne and we agree this shouldn't be an issue as we'll support either net6 or net7, and can fix anything up when we bump.

@lambdageek lambdageek merged commit acb0050 into dotnet:main Sep 30, 2021
radical added a commit to radical/runtime that referenced this pull request Oct 2, 2021
On VS2022 Preview 5, building a blazorwasm project which used sqlite
native library crashed:

```
System.BadImageFormatException: Expected signature header for 'Property' or 'Method', but found '9' (0x09).

 at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.CheckMethodOrPropertyHeader(SignatureHeader header)
 at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeMethodSignature(BlobReader& blobReader)
 at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeType(BlobReader& blobReader, Boolean allowlypeSpecifications, Int32 typeCode)
 at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeMethodSignature(BlobReader& blobReader)
 at System.Reflection.Metadata.MethodDefinition.DecodeSignature[TType, TGenericContext] (ISignatureTypeProvider`2 provider, TGenericContext genericCo
 at System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.SpecializeMethodSigStrings(TypeContext& typeContext)
 at System.Reflection.TypeLoading.RoDefinitionMethod`1.ComputeMethodSigStrings()
 at System.Reflection.TypeLoading.RoMethod.ToString()
 at System.Text.StringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ParamsArray args)
 at System.String.FormatHelper(IFormatProvider provider, String format, ParamsArray args)
 at System.String.Format(String format, Object arg0, Object arg1, Object arg2)
 at PInvokeComparer.GetHashCode(PInvoke pinvoke)
 at ...
 at System.Ling.Enumerable.<DistinctIterator>d__64`1.MoveNext()
 at System.Ling.Buffer`1..ctor(IEnumerable`1 source)
 at System.Ling.Enumerable.ToArray [TSource] (IEnumerable`1 source)
 at PInvokeTableGenerator.EmitPInvokeTable(StreamWriter w, Dictionary`2 modules, List`1 pinvokes)
 at PInvokeTableGenerator.GenPInvokeTable(String[] pinvokeModules, String[] assemblies)
 at PInvokeTableGenerator.Execute()
```

- This is crashing in a new code path(`PInvokeComparer.GetHashCode`) that
  invokes `methodInfo.ToString()` for methods with `DllImport` attribute.
- It fails for `EnumCalendarInfo`, which has a function pointer parameter,
  which `System.Reflection.MetadataLoadContext`(SRMLC) does not support, and is
  explicitly worked around in the code.
- It usually fails with on `methodInfo.GetParameters()` with
  `NotSupportedException`, and `methodInfo.ToString()` works fine

The difference seems to be because the version of `SRMLC` being bundled
with the task is `1.4.5.0`, which is too old.

The fix is to use the same version of SRMLC, and
`System.Reflection.Metadata`(SRM) used by other tasks, which is `5.0.0`
currently.

Also, this mirrors some other reference changes from dotnet#59720

- don't include msbuild assemblies with the task
- and explicitly reference `System.Text.Json`, and
  `System.Threading.Tasks.Extensions` to be sure about the version that
  we'll include for net472

The following in a table of the assemblies that are bundled with the task, for `net472`, and `net6.0`.
The `new` has fewer assemblies.

| Path                                                          | Old         | New         |
|---------------------------------------------------------------|-------------|-------------|
| net472/Microsoft.Bcl.AsyncInterfaces.dll                      | 1.0.0.0     | 5.0.0.0     |
| net472/Microsoft.Build.Framework.dll                          | 15.1.0.0    |             |
| net472/Microsoft.Build.Tasks.Core.dll                         | 15.1.0.0    |             |
| net472/Microsoft.Build.Utilities.Core.dll                     | 15.1.0.0    |             |
| net472/Microsoft.Build.dll                                    | 15.1.0.0    |             |
| net472/Microsoft.NET.StringTools.dll                          | 1.0.0.0     |             |
| net472/Microsoft.VisualStudio.Setup.Configuration.Interop.dll | 1.0.0.0     |             |
| net472/System.Buffers.dll                                     | 4.0.3.0     | 4.0.3.0     |
| net472/System.Collections.Immutable.dll                       | 5.0.0.0     | 5.0.0.0     |
| net472/System.Configuration.ConfigurationManager.dll          | 4.0.3.0     |             |
| net472/System.Memory.dll                                      | 4.0.1.1     | 4.0.1.1     |
| net472/System.Numerics.Vectors.dll                            | 4.1.4.0     | 4.1.4.0     |
| net472/System.Reflection.Metadata.dll                         | 1.4.5.0     | 5.0.0.0     |
| net472/System.Reflection.MetadataLoadContext.dll              | 4.0.1.1     | 5.0.0.0     |
| net472/System.Resources.Extensions.dll                        | 4.0.0.0     |             |
| net472/System.Runtime.CompilerServices.Unsafe.dll             | 5.0.0.0     | 5.0.0.0     |
| net472/System.Security.AccessControl.dll                      | 4.1.3.0     |             |
| net472/System.Security.Permissions.dll                        | 4.0.3.0     |             |
| net472/System.Security.Principal.Windows.dll                  | 4.1.3.0     |             |
| net472/System.Text.Encodings.Web.dll                          | 4.0.5.0     | 5.0.0.0     |
| net472/System.Text.Json.dll                                   | 4.0.1.0     | 5.0.0.0     |
| net472/System.Threading.Tasks.Dataflow.dll                    | 4.6.3.0     |             |
| net472/System.Threading.Tasks.Extensions.dll                  | 4.2.0.0     | 4.2.0.1     |
| net472/System.ValueTuple.dll                                  | 4.0.3.0     | 4.0.3.0     |
| net472/WasmAppBuilder.dll                                     | 6.0.0.0     | 6.0.0.0     |
|                                                               |             |             |
| net6.0/System.Reflection.MetadataLoadContext.dll              | 4.0.1.1     | 5.0.0.0     |
| net6.0/WasmAppBuilder.dll                                     | 6.0.0.0     | 6.0.0.0     |
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants